Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Omit unnecessary field from location range #1788

Merged
merged 4 commits into from
Sep 6, 2024

Conversation

pocke
Copy link
Member

@pocke pocke commented Apr 5, 2024

This PR reduces memory usage for RBS::Location by omitting unnecessary fields.

Previously RBS::Location had byte pos, char pos, line, and column. But they are caches except for char pos. These caches are rarely accessed. So we can drop them from RBS::Location.

design doc: https://hackmd.io/QNkMU6roTqSn_iXb2ENhsA

Benchmarking

I used benchmark/memory_new_rails_env.rb for the benchmark.
It decreases 15% memory.

# Before
Total retained:  52372539 bytes (459882 objects)

allocated memory by class
-----------------------------------
 152307312  Hash
  20146528  RBS::Location
  15744181  String
  15124312  Array


# After
Total retained:  44234355 bytes (459882 objects)

allocated memory by class
-----------------------------------
 152307312  Hash
  15744181  String
  15124312  Array
  12008296  RBS::Location

This PR also fixes two problems of the current implementations.

First, Buffer#pos_to_loc returns the wrong line and column for the EOS position of a string without the end of a newline. For example, the location for abc| (| is the cursor) should be [1, 3], but #pos_to_loc unexpectedly returns [2, 0].
This problem is fixed in 81b44e1

Second, #pos_to_loc returns an unexpected value when the position is located EOF.
The cause is that the parser determines the length of EOF as 1. Then, the RBS::Location of the pEOF token contains an out-of-range position. Because an EOF has a length in C String, but it doesn't have a length in Ruby String.
I changed the length of EOF token to zero to fix this problem.
16006c2

@pocke pocke force-pushed the Omit_unnecessary_field_from_location_range branch 2 times, most recently from 775f7dc to f752339 Compare April 15, 2024 05:37
@pocke pocke force-pushed the Omit_unnecessary_field_from_location_range branch from f752339 to 2c127f2 Compare September 5, 2024 06:59
@pocke pocke force-pushed the Omit_unnecessary_field_from_location_range branch from 2c127f2 to fc3230c Compare September 6, 2024 02:15
@pocke pocke force-pushed the Omit_unnecessary_field_from_location_range branch from 80a4654 to 553543d Compare September 6, 2024 06:28
@pocke pocke marked this pull request as ready for review September 6, 2024 06:41
@soutaro soutaro added this to the RBS 3.6 milestone Sep 6, 2024
@soutaro
Copy link
Member

soutaro commented Sep 6, 2024

Great work! 👏

RBS::Location now stores rbs_loc_range, a pair of start char pos and end char pos, but other C internal code keeps using range, with additional fields. Because the range is internal and not exposed as Ruby objects, this PR reduces the memory usage after parsing. 👍

Copy link
Member

@soutaro soutaro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎉

@pocke pocke added this pull request to the merge queue Sep 6, 2024
Merged via the queue into ruby:master with commit 9777d42 Sep 6, 2024
19 checks passed
@pocke pocke deleted the Omit_unnecessary_field_from_location_range branch September 6, 2024 07:40
@soutaro soutaro added the Released PRs already included in the released version label Sep 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Released PRs already included in the released version
Development

Successfully merging this pull request may close these issues.

2 participants